Skip to content

[exporter/elasticsearch] use validation for batcher #38072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Feb 27, 2025
Merged

[exporter/elasticsearch] use validation for batcher #38072

merged 10 commits into from
Feb 27, 2025
+56 −8

Conversation

@VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Feb 20, 2025 •
edited
Loading

Description

This PR fixes the validation for batcher.

  • We create our own batcher struct and embed it, instead of using exporterbatcher.Config (for backward compatibility). We ignore key validations done in exporterbatcher's validate.

Testing

Added

Documentation

@VihasMakwana VihasMakwana requested a review from a team as a code owner February 20, 2025 12:18
@VihasMakwana VihasMakwana mentioned this pull request Feb 20, 2025
Merged
@VihasMakwana VihasMakwana requested a review from axw February 20, 2025 12:25
@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 20, 2025

cc: @axw as you introduced batching in exporter.

carsonip
carsonip suggested changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@carsonip carsonip left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While choosing sync vs async indexer, we use config.Enabled != nil. This doesn't really take the boolean stored in Enabled into account. It just checks if pointer is nil or not. It can be possible if pointer is non-nil and *config.Enabled is false.

It was done so for backwards compatibility. See README: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter#batching

We needed to retain the existing behavior of async bulk indexer, as exporterhelper batcher wasn't good enough to become the default at the time ( 😿 look at the discussions in #32632 dragged on for months). We should have a separate discussion whether it is good enough to completely replace async bulk indexer.

If the motivation of the PR is to validate the batcher struct, the way to go is to check for non-nil batcher config and call validate func on it.

@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 20, 2025 •
edited
Loading

@carsonip Thanks for your comment. I'll revert changes and call validate on it.
Can you comment regarding the 2nd point in my PR i.e. comparison of pointer?

@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 20, 2025

FWIW, i think we can only focus on validation of batcher for this PR. And discuss other points in another followup issue.

@carsonip
Copy link
Contributor

carsonip commented Feb 20, 2025

Can you comment regarding the 2nd point in my PR i.e. comparison of pointer?

It is also intended. Any non nil batcher config disables async bulk indexer. i.e. if batcher config enabled is false, there will be no batching, and any request is blocked until indexed. if batcher config enabled is true, there is batching, and also blocks until the batch is indexed. There is only 1 case where batcher config is non-nil where the behavior is async, which is when sending queue is enabled.

@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 20, 2025

I see. I think I may have misunderstood the nuances. I assumed different stuff.
thanks!

@VihasMakwana VihasMakwana marked this pull request as draft February 20, 2025 12:59
@VihasMakwana VihasMakwana force-pushed the fix-batcher-elasticsearchexporter branch from e5f0dfc to b29fff8 Compare February 20, 2025 13:25
@VihasMakwana VihasMakwana marked this pull request as ready for review February 20, 2025 13:26
@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 20, 2025 •
edited
Loading

@carsonip I've only kept validation for this PR. PTAL!

@VihasMakwana VihasMakwana mentioned this pull request Feb 20, 2025
Open
@VihasMakwana VihasMakwana changed the title [exporter/elasticsearch] use collector's batcher config instead of our own [exporter/elasticsearch] use validation for batcher Feb 20, 2025
carsonip
carsonip approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@carsonip carsonip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

carsonip
carsonip reviewed Feb 20, 2025
View reviewed changes
axw
axw approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a nit about duplication

@VihasMakwana VihasMakwana requested a review from mwear February 21, 2025 08:49
@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 21, 2025

@carsonip @mwear @axw I've made slight change and introduced a helper. Can you again take a look once you have a moement?

axw
axw approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a nit :)

@VihasMakwana VihasMakwana force-pushed the fix-batcher-elasticsearchexporter branch from 425e80c to 4b4f1b1 Compare February 21, 2025 08:55
@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 26, 2025

Can someone from @open-telemetry/collector-contrib-approvers @open-telemetry/collector-contrib-maintainers approve this and merge it? It's already approved by codeowners but needs an approval from approvers/maintainers.

@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Feb 27, 2025

@open-telemetry/collector-contrib-maintainers can we merge this ?

@songy23 songy23 merged commit 20a49b7 into open-telemetry:main Feb 27, 2025
160 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 27, 2025
@songy23
Copy link
Member

songy23 commented Feb 27, 2025

unfortunately this broke mainline http://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/13571662124/job/37938103918

songy23 added a commit that referenced this pull request Feb 27, 2025
@songy23
Revert "[exporter/elasticsearch] use validation for batcher (#38072)"
0156710 
This reverts commit 20a49b7.
@songy23 songy23 mentioned this pull request Feb 27, 2025
Merged
songy23 added a commit that referenced this pull request Feb 27, 2025
@songy23
Revert "[exporter/elasticsearch] use validation for batcher" (#38254)
765ed69 
Reverts #38072

breaks mainline
http://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/13571662124/job/37938103918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@axw axw axw approved these changes

@dehaansa dehaansa Awaiting requested review from dehaansa dehaansa is a code owner automatically assigned from open-telemetry/collector-contrib-approvers

@JaredTan95 JaredTan95 Awaiting requested review from JaredTan95

@lahsivjar lahsivjar Awaiting requested review from lahsivjar

@mwear mwear Awaiting requested review from mwear

+1 more reviewer

@carsonip carsonip carsonip approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mwear mwear

@evan-bradley evan-bradley

Labels

exporter/elasticsearch

Projects

None yet

Milestone

v0.121.0

Development

Successfully merging this pull request may close these issues.

7 participants

@VihasMakwana @carsonip @songy23 @axw @mwear @evan-bradley @mx-psi